Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-715504: MFA token cache support #988

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

sfc-gh-jmartinezramirez
Copy link
Collaborator

@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez commented Jul 4, 2024

Description

Added support for MFA token cache.

Checklist

  • Code compiles correctly
  • Code is formatted according to Coding Conventions
  • Created tests which fail without the change (if possible)
  • All tests passing (dotnet test)
  • Extended the README / documentation, if necessary
  • Provide JIRA issue id (if possible) or GitHub issue id in PR name

@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 5 times, most recently from 6eba99f to 69d80c7 Compare July 5, 2024 21:54
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez changed the title Snow 715504 mfa token cache SNOW-715504: MFA token cache support Jul 8, 2024
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 91.51376% with 37 lines in your changes missing coverage. Please review.

Project coverage is 86.41%. Comparing base (5e16693) to head (7d959a3).

Files with missing lines Patch % Lines
Snowflake.Data/Core/Session/SFSession.cs 78.94% 6 Missing and 2 partials ⚠️
...e.Data/Client/SnowflakeCredentialManagerFactory.cs 89.09% 3 Missing and 3 partials ⚠️
Snowflake.Data/Core/Tools/UnixOperations.cs 71.42% 3 Missing and 3 partials ⚠️
...astructure/SFCredentialManagerWindowsNativeImpl.cs 92.72% 2 Missing and 2 partials ⚠️
Snowflake.Data/Core/Session/SessionPool.cs 92.00% 0 Missing and 4 partials ⚠️
...ager/Infrastructure/SFCredentialManagerFileImpl.cs 97.75% 0 Missing and 2 partials ⚠️
Snowflake.Data/Core/Tools/StringUtils.cs 75.00% 1 Missing and 1 partial ⚠️
...Core/Authenticator/ExternalBrowserAuthenticator.cs 0.00% 1 Missing ⚠️
...nowflake.Data/Core/Authenticator/IAuthenticator.cs 95.83% 0 Missing and 1 partial ⚠️
...ke.Data/Core/Authenticator/KeyPairAuthenticator.cs 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
+ Coverage   86.11%   86.41%   +0.30%     
==========================================
  Files         120      126       +6     
  Lines       11628    12028     +400     
  Branches     1167     1217      +50     
==========================================
+ Hits        10013    10394     +381     
- Misses       1332     1335       +3     
- Partials      283      299      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Snowflake.Data/Core/Authenticator/IAuthenticator.cs Outdated Show resolved Hide resolved
@@ -116,13 +134,19 @@ private SFRestRequest BuildLoginRequest()
{
loginName = session.properties[SFSessionProperty.USER],
accountName = session.properties[SFSessionProperty.ACCOUNT],
// TODO LOCAL TEST MFA temp change should be removed before merge
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would we need to pretend to be a jdbc driver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was only for testing it should use .NET before complete the PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just test as .net driver? Is there any particular reason to pretend to be a jdbc driver?
Anyway we need to remove it before merging the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we will do it before merging

Snowflake.Data/Core/Authenticator/MFACacheAuthenticator.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 4 times, most recently from 6af9d1a to e36f8ed Compare September 4, 2024 13:31
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 2 times, most recently from e795b5f to 9316bbc Compare September 18, 2024 23:08
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez marked this pull request as ready for review October 4, 2024 02:42
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez requested a review from a team as a code owner October 4, 2024 02:42
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SFSession.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Tools/StringUtils.cs Outdated Show resolved Hide resolved
Snowflake.Data.Tests/UnitTests/SFSessionTest.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Tools/UnixOperations.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/SFError.cs Outdated Show resolved Hide resolved
Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs Outdated Show resolved Hide resolved
Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SFSessionProperty.cs Outdated Show resolved Hide resolved
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 3 times, most recently from 5d32b9c to be6f19d Compare October 19, 2024 03:51
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SFCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs Outdated Show resolved Hide resolved
Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs Outdated Show resolved Hide resolved
…word_mfa authenticator.

Added mechanism to disable or throw an error if using a different authenticator using passcode in connection.
…ection string without username_password_authentication an exception will be thrown to indicate the user

that the passcode should not be used if pooling is enabled or with a minimum pool size greater than 0.
Additionally, if the passcode is provided by an argument and not part of the connection string, it will not be used for the session created by the session pool, and the push MFA mechanism will be triggered.
(cherry picked from commit ac28924)
…n is expired or invalid. Multi-factor authentication (MFA) will try to use the passcode from the connection string if available; otherwise, send a Duo push notification to try to authenticate again; if it fails, the token will be removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants